-
Notifications
You must be signed in to change notification settings - Fork 23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Wait for outbound capacity before paying jit channel open fee #1015
Conversation
9f22690
to
9418f0c
Compare
0bc00bc
to
b888ba0
Compare
51cd76c
to
e916e13
Compare
e916e13
to
d37b56f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Try to use tracing
fields wherever you can though 😛
mobile/native/src/channel_fee.rs
Outdated
}) | ||
.await | ||
.map_err(|e| anyhow!("{e:#}")) | ||
.context(format!( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔧 Should use with_context
instead.
.context(format!( | |
.with_context(|| format!( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well.. happy to change it, but do you think printing the text is such a huge performance cost, that we have to do it lazily? or do you have other reasons for suggesting that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sure it's negligible, but I think it's more correct to use with_context
when you have to build the error so that it is evaluated lazily.
So obviously not a big deal at all, but I wouldn't know why not to do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@holzeis just pointing out that I didn't mean to say to always use with_context
. It depends on the... context 😛
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
format!
itself also incurs an allocation since it makes a String
d37b56f
to
3c68e20
Compare
3c68e20
to
22932d2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it works, I'd say let's get it in.
bors r+ |
Build succeeded! The publicly hosted instance of bors-ng is deprecated and will go away soon. If you want to self-host your own instance, instructions are here. If you want to switch to GitHub's built-in merge queue, visit their help page. |
fixes #883